PP-4542 F-012: keep a Settings support path when the triage bot is disabled#1054
Conversation
🏗️ CodeAtlas Ledger Analysis✅ All Checks Passed♿ Accessibility (via AccessLint)
✅ No accessibility issues detected 🧪 Test Coverage (via QAAtlas)
🏛️ Architecture Analysis
🔍 Reachability Analysis
✅ No dead code detected 📊 0 files analyzed | 📦 Download Full Report Powered by CodeAtlas Ledger |
🧪 Unit Test Results📊 View Full Interactive Report ✅ ALL TESTS PASSED7129 tests | 7028 passed | 0 failed | 101 skipped | ⏱️ 10m 27s | 📊 98.6% | 📈 46.9% coverage Tests by Class — 821 classes (click to expand)
📊 Testing Coverage BreakdownUnit Test Line Coverage (testable surfaces): 46.9% Total coverage incl. UI/lifecycle: 45.2% (18 files excluded from testable denominator — see
🔗 Interactive HTML Report | CI Run Details 📦 Downloadable Artifacts
|
…between tests (#1056) `AccountsManager.deferInitialLoadCatalogsForTesting` is a process-wide mutable static flag gating whether `AccountsManager.init` spawns its background `loadCatalogs` Task. The test-safe value is `true` (no background work); `PalaceTestSetup.bootstrap()` pins it `true` at bundle load. Tests that need the background load opt IN by setting it `false` in their own setUp. Two sites left it `false` after running, leaking the unsafe value to the next test class — which then constructs a real `AccountsManager`, spawning the 1100+-library registry crawl that outlives the test and pollutes whatever runs next: - `AppContainer._resetForTesting()` — runs after EVERY test via the singleton-reset observer; its final line set the flag false. - `AppContainerResetTests.tearDown()` — explicitly set it false "for production semantics." Both now leave the flag at the test-safe `true`. This is the root cause behind recurring CI flakes that blocked PRs #1052/#1053/#1054 (none of whose own diffs were at fault): - AccountsManagerCancellationTests.testCancelBackgroundWork_onOptOutInstance _isSafeNoOp — opt-out handle non-nil (failed all 3 iterations) - CatalogCacheKeyAndIsolationTests — layout-engine-off-main crash - BookReturnCleverReauthTests — crash → ** TEST FAILED ** with 0 reported failures - TokenRefreshOnForeground / TokenRefreshAndRetryQueue — token/network bleed Regression: AppContainerResetTests.testResetForTesting_disablesBackgroundLoad Catalogs now asserts the flag is `true` post-reset (was asserting false) — the mutation that reintroduces the leak fails it. No production runtime change: the flag is only ever non-default inside an XCTest process (initializer keys off XCTestConfigurationFilePath); `_resetForTesting()` already early-returns outside XCTest. Verified: AppContainerResetTests + AccountsManagerCancellationTests + CatalogCacheKeyAndIsolationTests + TokenRefreshOnForegroundTests + TokenRefreshAndRetryQueueTests + BookReturnCleverReauthTests run together → 41 tests, 0 failures, ** TEST SUCCEEDED **. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
30ebb21 to
75a11cc
Compare
…+ legacy email when triage bot OFF `TPPSettingsView.supportSection` only rendered a Support section when `RemoteFeatureFlags.shared.isTriageBotEnabled`. The production Firebase default for `triage_bot_enabled` is FALSE, so in production the Support section vanished entirely, leaving users with no app-level support path. Add a `SupportSectionDecision` seam — a pure, fixture-free decision that maps (isTriageBotEnabled, currentAccount/supportEmail) to `.triageBot` or `.legacyEmail(address:)`. The section now ALWAYS renders: - bot ON: unchanged "Get Help" -> TriageBotSupportView row (byte-for-byte). - bot OFF: legacy "Report an Issue" row -> ProblemReportEmail.beginComposing, addressed to the current account's supportEmail, else the general fallback support@thepalaceproject.org. Empty/nil email also falls back, never a no-op. beginComposing already alerts when mail isn't configured, so the row is always safe to offer. Tests (PalaceTests/Settings/SupportSectionDecisionTests) pin all four branches as behavior assertions, incl. the critical no-supportEmail-still- reachable case. Hand mutation-verified: removing the `!email.isEmpty` guard kills the empty-email test; inverting the bot gate kills 4/5. **Scope:** Palace/Settings/NewSettings/TPPSettingsView.swift (seam + else branch + presentLegacyReportIssue/topViewController helpers) and the new test file. No change to ProblemReportEmail, RemoteFeatureFlags, or FirebaseManager. Bot-ON path verified byte-for-byte unchanged. **Not done:** no simdrive E2E pass (SwiftUI body not driven); decision covered by unit tests at the production seam. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
75a11cc to
bf298a1
Compare
Problem
TPPSettingsView.supportSectionwrapped the entire main-Settings "Support" section inif RemoteFeatureFlags.shared.isTriageBotEnabledwith noelse. Thetriage_bot_enabledFirebase flag defaults to false in production (FirebaseManager.swift:105), so in production the Support section never appears — and a patron on a library without asupportEmail/supportURLhas no in-app support path at all. A kill-switch should degrade to legacy support, not delete the entry point.(Not a strict regression vs 3.1.0 — main Settings had no Support section then either — but #1032 shipped a support feature that is invisible in production with no fallback. Caught during PP-4542 manual review.)
Fix
Introduced a pure, testable seam
SupportSectionDecision.decide(isTriageBotEnabled:supportEmail:):.triageBot(the existing "Get Help" →TriageBotSupportViewrow, byte-for-byte unchanged)..legacyEmail(account.supportEmail ?? "support@thepalaceproject.org")→ a "Report an Issue" row that opens the legacyProblemReportEmailcompose flow (which already handles the no-mail-configured case with an alert).The Support section now always renders, so support is reachable for every library/config.
Tests (
SupportSectionDecisionTests, 5).triageBotregardless of email (legacy not shown)support@thepalaceproject.org(the critical "support still reachable" case)** TEST SUCCEEDED **(5/5). Mutation surface isif let/bool (not auto-mutable); hand-verified by neutralizing the fallback and the flag check → tests fail. DoD checks (name-vs-body, blast-radius, superpartner) exit 0. Bot-ON path confirmed unchanged via diff.Jira: PP-4542 (F-012)